Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Haskell checks on ci #5

Merged

Conversation

sashasashasasha151
Copy link
Member

@sashasashasasha151 sashasashasasha151 commented Feb 17, 2021

Description

Related issue(s)

https://issues.serokell.io/issue/EDNA-11

✅ Checklist for your Pull Request

Related changes (conditional)

  • Tests

    • If I added new functionality, I added tests covering it.
    • If I fixed a bug, I added a regression test to prevent the bug from
      silently reappearing again.
  • Documentation

    • I checked whether I should update the docs and did so if necessary:

Stylistic guide (mandatory)

backend/default.nix Outdated Show resolved Hide resolved
flake.lock Outdated Show resolved Hide resolved
.buildkite/pipeline.yml Outdated Show resolved Hide resolved
@sashasashasasha151 sashasashasasha151 force-pushed the sashasashasasha151/edna11-haskell-checks-on-ci branch 2 times, most recently from d8c00a5 to 62c2d29 Compare February 19, 2021 15:00
@gromakovsky
Copy link
Member

We've decided not to do the check for cabal files in this PR (unless we find a solution quickly).

@sashasashasasha151 sashasashasasha151 force-pushed the sashasashasasha151/edna11-haskell-checks-on-ci branch 2 times, most recently from 2d31644 to 4a6e6d2 Compare February 20, 2021 17:22
@sashasashasasha151 sashasashasasha151 force-pushed the sashasashasasha151/edna11-haskell-checks-on-ci branch from 5a660a7 to 093f5a6 Compare February 20, 2021 17:28
.buildkite/pipeline.yml Outdated Show resolved Hide resolved
backend/.#default.nix Outdated Show resolved Hide resolved
backend/default.nix Outdated Show resolved Hide resolved
);

# Set a constant name for the src closure
source = let
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU this source thing and the above ignoreFilter are only used to run hlint, I see it used only in hlint-html (and btw, does ${source} there refer to this source or something else?). Is it indeed needed only for hlint or is it implicitly used somewhere else? I just Ctrl-F'ed source.

I have an alternative solution: just call hlint from pipeline.yaml using nix run. Pros:

  1. No need to have all this extra code in this file (source, ignoreFilter). Though maybe I am wrong and it's needed for something else as well.
  2. Less steps involved, easier to understand what's going on, easier to find what arguments are passed and where to change them.

AFAIU there is one problem: we don't know how to make nix run work with flakes. If that's the case and we don't figure it out quickly, I am okay with changing it later.

library = project.edna.components.library;
server = project.edna.components.exes.edna-server;
test = project.edna.checks.edna-test;
hlint-html = runCommand "hlint.html" {} "${hlint}/bin/hlint ${source} --no-exit-code --report=$out -j";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What is hlint.html? Do you understand?
  2. Why do we pass --no-exit-code? This way it seems very unlikely to notice when it reports anything. If we don't want CI to fail when hlint fails, we can mark the corresponding step as optional in the CI config. Just set soft_fail: true. I think it's better because this way it will be easier to notice that hlint reported something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It's the name of the output file generated in the closure for the hlint-html derivation. It's an arbitrary name. The important part is the closure itself and its hash, which depends on the inputs defined in source.

  2. No idea. This looks like what Yegor wrote years ago for Disciplina. I suspect it has just been cargo culted ever since. Feel free to adjust the invocation of hlint as you see fit. Just make sure you put something in $out somehow.

pipeline.nix Outdated Show resolved Hide resolved
flake.nix Outdated
backend-hlint = backend.hlint;
}) nixpkgs.legacyPackages;

pipelineFile = common-infra.mkPipelineFile self;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And do we need this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, actually.

flake.nix Outdated
let pkgs = nixpkgs.legacyPackages.${system};
in pkgs.mkShell {
inputsFrom = [ packages.backend-lib ];
buildInputs = with pkgs.haskellPackages; [ cabal-install hpack ];
Copy link
Member

@gromakovsky gromakovsky Feb 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use dev shell and I am not sure anybody needs it, but if we decide to keep it, probably we don't need hpack here because we don't use it (no package.yaml).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

devShell is the Flake equivalent of shell.nix -- and yes, I use it all the time.

flake.nix Outdated
outputs = { self, nixpkgs, haskell-nix, hackage, stackage, common-infra }:
let
packagesFor = system: rec {
pkgs = nixpkgs.legacyPackages.${system}.extend
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the semantic of this pkgs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flakes define derivations for all known systems. In the context of a flake, nixpkgs isn't a set of packages. It's a set of sets of packages. So you need to use the set for the current (for some definition of "current") system.

flake.nix Outdated
with packagesFor system; {
backend-lib = backend.library;
backend = backend.server // {
meta.modulePath = [ "services" "edna" "backend" ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is meta.modulePath?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea. Removed it.

flake.nix Outdated
};
}) nixpkgs.legacyPackages;

ciSystems = [ "x86_64-linux" ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, it seems very unlikely that we will have more than one ciSystem in any foreseeable future, so this file can be simplified, we don't need to iterate over systems and pass them to functions, we can have a single constant ciSystem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used anywhere. mkPipelineFile only generates steps for x86_64-linux.

flake.lock Outdated
"type": "github"
}
},
"naersk": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/nmattia/naersk says "Nix support for building cargo crates". Do we need it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used by deploy-rs. flake.lock contains all transitive dependencies for all inputs in the flake. deploy-rs is a transitive dependency of common-infra which is an input to this flake.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIU we don't use deploy-rs either, but I see that it's a dependency of common-infra, so ok. It's a bit poor granularity (that we need to mention so many packages just for a small piece of common-infra), but I guess we will end up using deploy-rs anyway, so it's not a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common-infra has 2 nix functions: mkNode to define deployment nodes, and mkPipelineFile that generates CI steps which uses the former to generate deployment steps. So we're technically using half of common-infra 😛

Packages mentioned in flake.lock don't necessarily end up in any of the closures, and since Nix is lazy, they don't even get evaluated unless you actually use them somewhere. The lockfile is eager and will contain all transitive locks regardless of whether they are used. It's just extra noise, that's all.

@gromakovsky
Copy link
Member

@mkaito I have a question to you.
In many repos we often do things like nix run -f ci.nix pkgs.reuse -c reuse lint. By passing pkgs.reuse here we ensure that reuse is available in $PATH. To make it work we need to define pkgs in ci.nix, so that it has reuse attribute. In ci.nix we have something like this:

  sources = import ./nix/sources.nix;
  haskellNix = import sources."haskell.nix" {
    sourcesOverride = { hackage = sources."hackage.nix"; stackage = sources."stackage.nix"; };
  };
  pkgs = import sources.nixpkgs haskellNix.nixpkgsArgs;

So AFAIU pkgs is nixpkgs and its revision is taken from nix/sources.json. That works well when we use niv.
I like this nix run -f ci.nix INSTALLABLE -c bla-bla-bla feature and I think we want to use it in this repo. But it's not clear what's the best way to do the same with flakes instead of niv.
First of all, since we have default.nix instead of ci.nix, we don't need to pass -f ci.nix, that's clear to me.
But it's not clear how to access nixpkgs like we have pkgs in the approach with niv. We have defaultNix in default.nix, so AFAIU its contents is fully defined by flake.nix. Since we have nixpkgs in flake's outputs, can we just write nix run nixpkgs.reuse? Or should we do it differently?

P. S. Example with niv: https://github.com/tqtezos/baseDAO

@gromakovsky
Copy link
Member

Since we have nixpkgs in flake's outputs

Ah, oh, sorry, it's not in outputs, it's in arguments of outputs, so outputs expects nixpkgs to be passed to it.

I've just tested my hypotheses. First I tried to just do nix run nixpkgs.hlint -c hlint, see here. It said:

attribute 'nixpkgs' in selection path 'nixpkgs.hlint' not found

Then I tried to also add nixpkgs = nixpkgs; (not sure if it makes sense), but got the same message here. You can also see commits I made, they are available from buildkite.

I don't want to go further with experiments, will wait for Chris to respond.

@mkaito
Copy link
Contributor

mkaito commented Feb 22, 2021

The command nix run changed semantics in 3.0. In 2.4 it was a replacement for nix-shell -p blah --run whatever, but in 3.0 it does something else entirely.

It relies on a flake concept called "apps", where you define executables in a flake output that can be run with nix run without installing. For example, you can nix run github:serokell/deploy-rs to run that program directly from git master without installing it.

What you're looking for is now called nix shell.

This will do what you want.

$ nix shell nixpkgs#hlint -c hlint

Note the nixpkgs#hlint reference. This puts the package hlint from the flake nixpkgs in $PATH and then calls command hlint. The nixpkgs flake is defined in the flake registry as github:nixos/nixpkgs.

Generally speaking, all our inputs should be declared in the flake, and everything should be run through the flake. The mkPipelineFile function will build a sepate CI step for each package, check, and deployment attribute in the flake, so you should just plug things into checks as necessary. I'm not sure it supports custom extra steps, but that can probably be added if necessary.

The files default.nix and shell.nix contain compatibility shims to allow Nix 2.4 to read flake outputs, so you can nix-build and nix-shell without installing 3.0.

@gromakovsky
Copy link
Member

The mkPipelineFile function will build a separate CI step for each package

What do you mean by "build a separate CI step"? Does it generate .pipeline.yml? Is it supposed to be appended to the main .pipeline.yml? Or replace it? Or what? How does it generate steps, how does it know what options to specify for steps, like soft_fail, if, etc.?

@gromakovsky
Copy link
Member

This will do what you want.

Ok, let me test it in my branch.

@mkaito
Copy link
Contributor

mkaito commented Feb 22, 2021

Does it generate .pipeline.yml?

Yes.

How does it generate steps

By reading the packages and checks outputs in the flake and generating one CI step for each. Here's what it currently generates (nix build .#pipelineFile && cat result | jq):

{
  "steps": [
    {
      "agents": [],
      "command": "nix-build -A packages.x86_64-linux.backend-lib",
      "label": "Build backend-lib"
    },
    {
      "agents": [],
      "command": "nix-build -A packages.x86_64-linux.backend-server",
      "label": "Build backend-server"
    },
    {
      "agents": [],
      "command": "nix-build --no-out-link -A checks.x86_64-linux.backend-hlint",
      "label": "backend-hlint"
    },
    {
      "agents": [],
      "command": "nix-build --no-out-link -A checks.x86_64-linux.backend-test",
      "label": "backend-test"
    },
    "wait"
  ]
}

how does it know what options to specify for steps

It doesn't. It just runs what you tell it to run. For example, checks.backend-hlint is defined in backend/default.nix as such:

{
  hlint-html = runCommand "hlint.html" {} "${hlint}/bin/hlint ${source} --no-exit-code --report=$out -j";
}

@gromakovsky
Copy link
Member

So does it replace .pipeline.yml? Can we refrain from doing that please and just write .pipeline.yml manually? I don't like it for many reasons:

  1. If I just want to do something simple on CI (e. g. call some .sh script), I need to go to some nix file and do it there. That's overhead and entry barrier.
  2. Developers generally don't like dealing with nix, so each use of nix should be justified. Here we just move some configuration from yaml to nix files, so I don't see a big benefit.
  3. Our CI approach changes every year, some details change every month. This mkPipelineFile thing is new (AFAIU) and not battle-tested (in many repos for a long time). I don't want to use such things in this project where we just need to quickly do what we know how to do and don't have much space for experiments. I am tired in this never-ending changes in the way we handle CI.
  4. If we want to specify custom agents or if or set soft_fail or any other option, is there any way to do it? AFAIU you are saying there is none.

For example, checks.backend-hlint is defined in backend/default.nix as such:

Yes, and I don't like that, don't understand why it's done this way and proposed to change that. I like

  - label: hlint
    commands:
      - nix shell nixpkgs#hlint -c hlint

much more.

@mkaito
Copy link
Contributor

mkaito commented Feb 22, 2021

I've cleaned up some things and removed pipeline generation.

@gromakovsky
Copy link
Member

@mkaito thank you, that's exactly what I wanted to see. LGTM now. One last question I wanted to ask: I see the things in flake's output are called packages and checks. Is it flake's conversion or our own? I was thinking that these names are treated specially by mkPipelineFile: it passes --no-out-link for checks and doesn't do it for packages. Is there anything else where these names matter?

If I rename packages to foo and checks to bar and update .pipeline.yml accordingly, will it work the same way? I am ok with current names and don't suggest changing them, just want to know whether there are any other conventions.

Copy link
Member

@gromakovsky gromakovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with merging it given that:

  1. You fix CI so that it passes (obviously).
  2. Commits are cleaned up.
  3. @sashasashasasha151 reviews latest changes and confirms he's satisfied with them.

@sashasashasasha151 sashasashasasha151 changed the title [EDNA-11] Haskell checks on ci Haskell checks on ci Feb 24, 2021
@mkaito
Copy link
Contributor

mkaito commented Feb 24, 2021

I see the things in flake's output are called packages and checks. Is it flake's conversion or our own?

There are outputs that are treated specially. packages are build outputs, while nix flake check (need nix 3.0) will eval everything referenced in the flake, build all packages and also all checks. There's other meaningful outputs, such as nixosModules and overlay. Flakes present a standardized interface to modular repositories of NixOS components. See doc links further down.

it passes --no-out-link for checks and doesn't do it for packages.

The result symlink act as a GC root. As long as something in the store is referenced by a gc root, it will not get collected. We pin closures for packages but not for tests. The rationale being that packages are used for deployment and we don't want to rebuild them if a GC happens at the wrong time.

Is there anything else where these names matter?

Yeah, although flakes are sparsely documented. Half of what we know we learned by reading the C++ source code of Nix itself. The best documentation I've found is oddly enough nix flake --help, for which you need a very recent version of the Nix client installed.

The de-facto standard documentation is the old and closed RFC 49. There's a half decent wiki page too, and some tutorial style blog posts.

The Nix folks keep pussyfooting about Flakes. Meanwhile, the rest of the Nix world has been using them a long time and are quite happy. They actually just moved the target for flake release from 3.0 to 3.5 for no apparent reason other than more bikeshedding. Never change, Nix. Never change...

If I rename packages to foo and checks to bar and update .pipeline.yml accordingly, will it work the same way?

Yes they will, because we don't use any flake specific features in the current pipeline. The shim in default.nix presents things in the "old" way, and we just build the derivations with the right names and that's it. This is necessary because our servers run stable nix. We do try however to make it as easy as possible to switch to flake-native Nix in the future, hopefully without having to rewrite every single pipeline. All of these compat shims are used now because flakes require unstable nix, and we don't want to run unstable nix daemon on our servers.

It becomes important once you use a Nix 3.0 client and want to use things like nix flake check, which would eval everything, build all packages (for all architectures!) and also all checks.

Some of the flakes I've written for various projects keep a pinned master build of Nix in the devShell so that we can run things like flake checks directly without a shim, but we haven't quite standardized on how exactly we want to do these things.

@mkaito mkaito force-pushed the sashasashasasha151/edna11-haskell-checks-on-ci branch from 91fbc0f to a2d7986 Compare February 24, 2021 14:56
@sashasashasasha151 sashasashasasha151 force-pushed the sashasashasasha151/edna11-haskell-checks-on-ci branch from a2d7986 to 5585fc3 Compare February 24, 2021 19:59
@sashasashasasha151 sashasashasasha151 marked this pull request as ready for review February 24, 2021 19:59
@sashasashasasha151 sashasashasasha151 merged commit 3dced9c into master Feb 24, 2021
@sashasashasasha151 sashasashasasha151 deleted the sashasashasasha151/edna11-haskell-checks-on-ci branch February 24, 2021 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants